Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix infinite def write #316

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kareefardi
Copy link

I think .def_part is resolved to .def resulting in infinite write behavior. This renames the partial file .._part.def avoiding the infinite loop while writing def.

I am not sure however if freeMagic(outName_part) is called in all the correct places.

Signed-off-by: Kareem Farid <[email protected]>
Copy link
Contributor

@dlmiles dlmiles Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider this slightl tweak to approach, outName can be NULL (according to function docs), the lefFileOpen() will return the actual filename used (after resolving what it will be, when not provided via outName). So you can't do things like strlen(NULL).

The codebase doesn't really have NULL checks on failed mallocMagic() calls, so this mitigated to ASSERT and removes the whole what to do when it fails consideration the if() block implies.

I'm assuming the filename contains the file extension (.def/.lef) as it is used directly with unlink() as-is.

So this moves the block to be lower down around line 2986 of the original file.

    filename1 = StrDup((char **)NULL, filename); // from original file line 2985

    char *outName_part;
    {   /* use the 'filename' given back from lefFileOpen() */
        outName_part = mallocMagic(strlen(filename)+strlen("_part")+1);
        ASSERT(outName_part, "outName_part");
        strcpy(outName_part, filename);
        char *cp = strrchr(outName_part, '.');
        if (cp)
           *cp = '\0'; /* remove extn */
        strcat(outName_part, "_part");
    }

    defWriteHeader(def, f, scale, units); // from original file line 2987 (remove this comment)

@RTimothyEdwards
Copy link
Owner

@dlmiles : My problem with the original pull request is that I still don't understand the nature of the infinite loop and I don't see how the code change fixes it, and I don't understand the need to change the file name being written out. This is mostly do to me having limited time to check and test code changes.

@dlmiles
Copy link
Contributor

dlmiles commented Oct 9, 2024

Understand, just trying to help out with the code review aspect to point out strlen(outName) is problematic because outName can be NULL. The returned variable filename looks a better variable to use.

I am not affected by the problem this commit is trying to address.

@kareefardi is the infinite loop an internal magic infinite loop? Or an issue with external tooling seeing or not seeing well formed files with the appropriate file extension?

Is there an example command sequence to run with project files to demonstrate the original problem?

@kareefardi
Copy link
Author

@dlmiles @RTimothyEdwards I was able to isolate the problem to it being caused by calling def write spm.def instead of def write spm. Here is a demonstration of this:
demo

@dlmiles
Copy link
Contributor

dlmiles commented Oct 14, 2024

diff --git a/lef/lefWrite.c b/lef/lefWrite.c
index 024fdf2a..639cef78 100644
--- a/lef/lefWrite.c
+++ b/lef/lefWrite.c
@@ -191,10 +191,12 @@ lefFileOpen(def, file, suffix, mode, prealfile)
     {
        if (strcmp(endp, suffix))
        {
+#if 0
            /* Try once as-is, with the given extension.  That takes care   */
            /* of some less-usual extensions like ".tlef".                  */
            if ((rfile = PaOpen(name, mode, NULL, Path, CellLibPath, prealfile)) != NULL)
                return rfile;
+#endif
 
            len = endp - name;
            if (len > sizeof namebuf - 1) len = sizeof namebuf - 1;

This tries a re-open of the given filename "spm.def" and this is a hit and is successful, so it returns a handle as-is.
This is due to the suffix=".def_part" not matching the "spm.def" extension.

When the given filename is "spm" I guess it can't see an extension to try to match, so this is doesn't try this "optimization"

@dlmiles
Copy link
Contributor

dlmiles commented Oct 18, 2024

Order of events:

DefWriteCell(outName="spm.def") // or outName="spm" which does not trigger issue
f = lefFileOpen(def, outName, ".def", "w", &filename);
## 'f' has data written into file

f2 = lefFileOpen(def, outName, ".def_part", "w", &filename);
## data is written into 'f2'  this is acts like a tmpfile I guess due to out-of-order data processing
## but it is in the same directory as the outName
## it is unclear (and maybe not important) if this file actually unique and gets created, it maybe truncating 'f' but both files have data written into them now

## f2 has data written into it
fclose(f2);

## f1 has more data written into it (call it 512M of data)

f2 = lefFileOpen(def, outName, ".def_part", "r", &filename);
## f2 is reopening the previously processed data, but this reopen actually opens the same file as 'f' not a seperate

## at this point:
## 'f2' is RO handle on 'spm.def' at filepos=0
## 'f'is RW handle on 'spm.def' at filepos=512M
while (fgets(line, sizeof line, f2) != NULL) fprintf(f, "%s", line);
fclose(f2);
## this while loop causes infinite write, as the file handles are 512M apart of the same file

The optimization (if that is a good term) in the #if 0 commented out section, uses the outName as-is and has suffix=NULL in call to PaOpen() so it ignores the passed suffix=".def_part" passed to lefFileOpen() so when the optimization is triggered it opens spm.def

Previously (before this PR) outName is the same value to all calls to lefFileOpen(), which then derives the endp (extension) from that it can see in outName. endp=".def" suffix=".def_part"

By modifying outName to append _def_partfor the tempfile it permutes the filename enough so it can not match the 'f' filehandle filename of spm.def

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants